refactor git-autograder to generalize bebavior and reduce indirections by SAN-MUYUN · Pull Request #21 · git-mastery/git-autograder

SAN-MUYUN · 2026-03-09T03:32:41Z

Fixes #5 , fixes #6 , fixes #8
refactor code to generalize behavior and reduce indirections

RFC:

Copilot

Pull request overview

Refactors git-autograder to expose tag-related functionality through the repo abstraction and adds small utilities for working with commits and underlying GitPython objects.

Changes:

Introduces GitAutograderTag and TagHelper to query local/remote tags and resolve tag→commit.
Extends the repo abstraction (GitAutograderRepoBase, GitAutograderRepo, NullGitAutograderRepo) with a tags helper.
Adds convenience accessors: GitAutograderCommit.message, CommitHelper.commit_from_message, and GitAutograderExercise.git_repo.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/git_autograder/tag.py	Adds a wrapper type for GitPython tag refs with convenience properties.
src/git_autograder/helpers/tag_helper.py	Adds tag querying utilities (local tags, remote tag names, tag→commit helpers).
src/git_autograder/repo/repo_base.py	Adds `tags` to the repo interface.
src/git_autograder/repo/repo.py	Wires `TagHelper` into the concrete repo implementation.
src/git_autograder/repo/null_repo.py	Adds `tags` property to the null repo (currently with an incorrect error message).
src/git_autograder/helpers/commit_helper.py	Adds helper to find a commit by message.
src/git_autograder/commit.py	Adds decoded `message` accessor for commits.
src/git_autograder/exercise.py	Adds `git_repo` property for accessing the underlying `Repo` when not ignored.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-09T05:01:30Z

src/git_autograder/repo/null_repo.py

+    @property
+    def tags(self) -> TagHelper:
+        raise AttributeError(
+            "Cannot access attribute files on NullGitAutograderRepo. Check that your repo_type is not 'ignore'."


The AttributeError message in tags is copy/pasted from files (it says "Cannot access attribute files"), which will mislead users. Update it to reference tags (and consider matching the ignore/local-ignore wording used elsewhere, e.g. GitAutograderExercise.git_repo).

Suggested change

"Cannot access attribute files on NullGitAutograderRepo. Check that your repo_type is not 'ignore'."

"Cannot access attribute tags on NullGitAutograderRepo. Check that your repo_type is not 'ignore'."

desmondwong1215

Left some comments.

desmondwong1215 · 2026-03-09T04:28:40Z

src/git_autograder/repo/null_repo.py

+    @property
+    def tags(self) -> TagHelper:
+        raise AttributeError(
+            "Cannot access attribute files on NullGitAutograderRepo. Check that your repo_type is not 'ignore'."


It should be tags instead of files in the error message.

desmondwong1215 · 2026-03-09T04:37:44Z

src/git_autograder/helpers/tag_helper.py

+        names: List[str] = []
+        seen = set()
+
+        for line in raw.splitlines():
+            parts = line.split()
+            if len(parts) != 2:
+                continue
+
+            ref = parts[1]
+            if not ref.startswith("refs/tags/"):
+                continue
+
+            name = ref[len("refs/tags/") :]
+            if name.endswith("^{}"):
+                name = name[:-3]
+
+            if name not in seen:
+                seen.add(name)
+                names.append(name)
+
+        return names


Is there any reason that we need both names and seen for this method? Is it for maintaining the order?
Actually, we don't need the names list; we can add valid names to seen, and convert the seen set to a list as the return value.

desmondwong1215 · 2026-03-09T15:42:46Z

src/git_autograder/helpers/file_helper.py

+            contents = [
+                line.strip() for line in input_file.readlines() if line.strip() != ""
+            ]
+            if contents != expected:


You are comparing a list[str] against str, which will always return False.

desmondwong1215 · 2026-03-09T16:05:52Z

src/git_autograder/helpers/tag_helper.py

+    def points_to(self, tag_name: str, commit: GitAutograderCommit) -> bool:
+        tag_commit = self.commit_or_none(tag_name)
+        return tag_commit is not None and tag_commit.hexsha == commit.hexsha


This method looks like a behaviour of GitAutograderTag instead of TagHelper to me.

desmondwong1215 · 2026-03-09T16:14:20Z

src/git_autograder/helpers/tag_helper.py

+    def commit_or_none(self, tag_name: str) -> Optional[GitAutograderCommit]:
+        t = self.tag_or_none(tag_name)
+        if t is None:
+            return None
+        return t.commit


Why do we need this method? We will check whether the tag exists before retrieving its commit in our verify logic.

SAN-MUYUN · 2026-03-15T03:18:05Z

@desmondwong1215 Really appreciate the feedbacks! I have updated the PR accordingly, feel free to test it out and review again.

desmondwong1215

Leave some comments. Everything else LGTM!

desmondwong1215 · 2026-03-15T07:53:57Z

src/git_autograder/helpers/file_helper.py

+        with (
+            open(given, "r") as given_file,
+            open(expected, "r") as expected_file,
+        ):


Is there any reason that we are not using file() or file_or_none() here?

desmondwong1215 · 2026-03-15T07:55:52Z

src/git_autograder/helpers/file_helper.py

+            contents = given_file.read().replace("\n", "")
+            expected_contents = expected_file.read().replace("\n", "")
+            return contents == expected_contents


In the current implementation, a\nbc and ab\nc will be equal, but their content should be different. Is this the intended implementation?

This is done intentionally, since we do not want to punish users because of variation in line breaks. As long as the actual content is the same, we are good with it.
Anyways, for easy future integration, I will add a boolean field for developers to decide whether they want exact content or not.

desmondwong1215 · 2026-03-15T07:57:21Z

src/git_autograder/helpers/tag_helper.py

+            if name not in names:
+                names.append(name)


Is the ordering of the tag important in this case, as using set will be more efficient?

Personally I find using of set is unnecessary, just like you pointed out in the previous review. For a single exercise, it is unlikely that we have large number of tags such that we will need to use a set to keep track of the tags. It would be good to keep the ordering in some cases.

SAN-MUYUN · 2026-03-22T13:48:45Z

@desmondwong1215 fixed the are_files_equal method accordingly.

src/git_autograder/branch.py

refactor git-autograder to generalize bebavior and reduce indirections

feb0f97

SAN-MUYUN marked this pull request as draft March 9, 2026 03:32

jovnc requested a review from Copilot March 9, 2026 04:58

Copilot started reviewing on behalf of jovnc March 9, 2026 04:58 View session

Copilot AI reviewed Mar 9, 2026

View reviewed changes

make generalisation in FileHelper

80d729e

desmondwong1215 reviewed Mar 9, 2026

View reviewed changes

further refactor gitautograder

87c80ec

SAN-MUYUN marked this pull request as ready for review March 14, 2026 18:49

desmondwong1215 reviewed Mar 15, 2026

View reviewed changes

update file comparison logic

77efbc4

SAN-MUYUN added 2 commits March 22, 2026 23:40

ensure file is opened using encoding=utf-8

4cf6b90

implement generalization for commit count

d57e8f4

desmondwong1215 reviewed Mar 23, 2026

View reviewed changes

src/git_autograder/branch.py Outdated Show resolved Hide resolved

Apply suggestion from @desmondwong1215

b701279

	"Cannot access attribute files on NullGitAutograderRepo. Check that your repo_type is not 'ignore'."
	"Cannot access attribute tags on NullGitAutograderRepo. Check that your repo_type is not 'ignore'."

Conversation

SAN-MUYUN commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

desmondwong1215 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SAN-MUYUN commented Mar 15, 2026

Uh oh!

desmondwong1215 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SAN-MUYUN Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SAN-MUYUN commented Mar 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SAN-MUYUN commented Mar 9, 2026 •

edited

Loading

SAN-MUYUN Mar 22, 2026 •

edited

Loading